Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow retrying Delivery::{ack, nack} #24

Merged
merged 2 commits into from
Feb 7, 2024

Conversation

svix-jplatte
Copy link
Member

@svix-jplatte svix-jplatte commented Feb 6, 2024

… by returning self on failure.

Also improve docs a little bit.

svix-gabriel
svix-gabriel previously approved these changes Feb 6, 2024
Copy link
Contributor

@svix-gabriel svix-gabriel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the motivation, but conceptually I actually like that ack/nack consumes the delivery, since a delivery shouldn't be modified after it's been successfully ack/nacked.

I think a better approach would be to embed Self in one of the return values in the Error condition. That way, you could only retry acks/nacks if those operations failed.

That might be a more complicated change though, so I'm fine approving as is.

@svix-jplatte
Copy link
Member Author

It's not much more complicated. And the server code has to change in weird ways in either case, FWIW.

@svix-gabriel
Copy link
Contributor

Lets try that then. If it ends up being too complicated, I can just merge this as is.

Copy link
Contributor

@svix-gabriel svix-gabriel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (if you update the PR title and message I can merge)

@svix-jplatte svix-jplatte changed the title Change Delivery::{ack, nack} to borrow self (mutably) Allow retrying Delivery::{ack, nack} Feb 7, 2024
@svix-jplatte
Copy link
Member Author

Right, missed that. Done.

@svix-gabriel svix-gabriel merged commit 823be74 into main Feb 7, 2024
2 checks passed
@svix-jplatte svix-jplatte deleted the jplatte/retry-delivery-ack branch February 7, 2024 14:54
svix-onelson added a commit to svix/svix-webhooks that referenced this pull request Feb 12, 2024
… implementation (#1188)

~~Depends on #1185 (merged),
svix/omniqueue-rs#24

I have deleted low-level tests for the in-memory queue since its basic
functionality should be tested inside omniqueue, not here. Do we have
tests for higher-level functionality that uses the in-memory queue
backend in some scenarios?

The redis and SQS implementation should probably be replaced by the
implementations found in Omniqueue in a follow-up PR, to keep this one
reasonably small.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants